Skip to content

[PROD RELEASE] - Q2 #823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 194 commits into
base: master
Choose a base branch
from
Open

[PROD RELEASE] - Q2 #823

wants to merge 194 commits into from

Conversation

kkartunov
Copy link
Contributor

  • QA bug fixes
  • Copilot Portal

@@ -19,6 +21,8 @@ const updateMemberValidations = {
status: Joi.any()
.valid(_.values(INVITE_STATUS))
.required(),
source: Joi.string()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating the source field to ensure it only accepts specific values, similar to how status is validated. This can help prevent invalid data from being accepted.

@@ -29,6 +33,7 @@ module.exports = [
permissions('projectMemberInvite.edit'),
(req, res, next) => {
const newStatus = req.body.status;
const source = req.body.source;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating the source field to ensure it contains expected values before using it further in the code. This can help prevent potential issues from unexpected input.

@@ -81,7 +86,7 @@ module.exports = [
.update({
status: newStatus,
})
.then((updatedInvite) => {
.then(async (updatedInvite) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of async in the .then function suggests that there might be an await operation inside the function. Please ensure that any asynchronous operations within this function are properly handled with await. If there are no asynchronous operations, consider removing async for clarity.

@@ -94,7 +99,7 @@ module.exports = [
if (updatedInvite.status === INVITE_STATUS.ACCEPTED ||
updatedInvite.status === INVITE_STATUS.REQUEST_APPROVED) {
return models.ProjectMember.getActiveProjectMembers(projectId)
.then((members) => {
.then(async (members) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using async in a .then() method can be confusing and is generally not recommended. Consider using await within an async function instead of chaining .then(). This can improve readability and maintainability of the code.

.addUserToProject(req, member)
.then(() => res.json(util.postProcessInvites('$.email', updatedInvite, req)))
.catch(err => next(err));
const t = await models.sequelize.transaction();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling for the transaction initialization. If models.sequelize.transaction() fails, it could lead to unhandled exceptions.

transaction: t,
});

await application.update({ status: nextApplicationStatus }, {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that application is not null before calling update on it. If findOne does not find a record, application will be null, leading to a runtime error.

transaction: t
});

const opportunity = await models.CopilotOpportunity.findOne({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that opportunity is not null before calling update on it. If findOne does not find a record, opportunity will be null, leading to a runtime error.

transaction: t,
});

const request = await models.CopilotRequest.findOne({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that request is not null before calling update on it. If findOne does not find a record, request will be null, leading to a runtime error.

// update the application if the invite
// originated from copilot opportunity
if (updatedInvite.applicationId) {
const allPendingInvitesForApplication = await models.ProjectMemberInvite.getPendingInvitesForApplication(invite.applicationId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling for getPendingInvitesForApplication. If it fails, it could lead to unhandled exceptions.

@@ -225,6 +225,23 @@ const projectServiceUtils = {
return _.intersection(roles, ADMIN_ROLES.map(r => r.toLowerCase())).length > 0;
},

/**
* Helper funtion to verify if user has project manager role

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the comment: 'funtion' should be corrected to 'function'.

const isMachineToken = _.get(req, 'authUser.isMachine', false);
const tokenScopes = _.get(req, 'authUser.scopes', []);
if (isMachineToken) {
if (_.indexOf(tokenScopes, M2M_SCOPES.CONNECT_PROJECT_ADMIN) >= 0) return true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider simplifying the condition by directly returning the result of the comparison: return _.indexOf(tokenScopes, M2M_SCOPES.CONNECT_PROJECT_ADMIN) >= 0;.

return _.get(res, 'data.result.content', []);
});
} catch (err) {
logger.debug(err, "error on getting role info");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using logger.error instead of logger.debug for logging errors to ensure they are appropriately flagged in the logs.

.map(r => r.id);
});
} catch (err) {
return Promise.reject(err);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error logging similar to the getRoleInfo method to capture and debug errors effectively.

@@ -34,6 +34,10 @@ module.exports = function defineProjectMember(sequelize, DataTypes) {
],
});

ProjectMember.associate = (models) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling or validation to ensure that the association with models.Project is correctly established. This can help prevent runtime errors if models.Project is undefined or if there are issues with the foreign key.

},
],
})
.then((copilotOpportunity) => {
const plainOpportunity = copilotOpportunity.get({ plain: true });
const formattedOpportunity = Object.assign({}, plainOpportunity,
req.log.info("authUser", req.authUser);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log statement req.log.info("authUser", req.authUser); may expose sensitive information about the authenticated user. Consider logging only non-sensitive information or ensuring that sensitive data is adequately protected.

const memberIds = plainOpportunity.project.members && plainOpportunity.project.members.map((member) => member.userId);
let canApplyAsCopilot = false;
if (req.authUser) {
canApplyAsCopilot = !memberIds.includes(req.authUser.userId)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic canApplyAsCopilot = !memberIds.includes(req.authUser.userId) assumes memberIds is always defined. Consider adding a check to ensure memberIds is not undefined before calling includes to avoid potential runtime errors.

if (req.authUser) {
canApplyAsCopilot = !memberIds.includes(req.authUser.userId)
}
// This shouldn't be exposed to the clientside

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment // This shouldn't be exposed to the clientside suggests that plainOpportunity.project.members should not be sent to the client. Ensure that the deletion of plainOpportunity.project.members is correctly implemented and tested to prevent accidental exposure.

return next();
}
req.log.info("token available", token);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging sensitive information such as tokens can lead to security vulnerabilities. Consider removing or masking the token in the log statement.

details: JSON.stringify({ message: 'You do not have permission to cancel copilot opportunity' }),
status: 403,
});
return Promise.reject(err);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Promise.reject(err) here is unnecessary because the function is already async. You can simply throw err to achieve the same effect.

return Promise.reject(err);
}
// default values
const opportunityId = _.parseInt(req.params.id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating opportunityId after parsing to ensure it is a valid integer and not NaN. This will prevent potential issues if the input is not a valid number.

transaction,
});

if (!opportunity) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message here could be more descriptive by including the opportunity ID in the log message for easier debugging.

transaction,
});

const applications = await models.CopilotApplication.findAll({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider checking if applications is empty before proceeding with the update to avoid unnecessary operations.

transaction,
});

res.status(200).send({ id: opportunity.id });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider sending a more detailed response, such as including a message indicating that the opportunity was successfully canceled, for better client-side handling.


// Cancel Copilot opportunity
router.route('/v5/projects/copilots/opportunity/:id(\\d+)/cancel')
.delete(require('./copilotOpportunity/delete'));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation for the .delete(require('./copilotOpportunity/delete')); line is inconsistent with the rest of the code. It should be indented to align with the .post(require('./copilotOpportunity/assign')); line above it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants